-
Notifications
You must be signed in to change notification settings - Fork 174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set consistent context in delayed_job integration #499
Conversation
This LGTM, though I'm going to confirm changing the potential context won't cause any issues first. |
b71652b
to
2f43c30
Compare
Hi @mike-stewart, I'm going to take it on from here to add some more end-to-end tests. Thanks for the contribution! |
@Cawllec Any ETA on this one? |
We have some longer term work going on adding breadcrumbs to the ruby notifier and we will slot this in after that. Might take a few weeks. You can always pin to your fork until we have got to merging this down if you need it sooner. |
Any plans to pull this @Cawllec @snmaynard @phillipsam? |
Hi @mike-stewart we are still planning to pull this when priorities allow. |
Thanks for the PR @mike-stewart! I've fixed the conflict and added an end-to-end test in #615 |
A fix for this has now been released in v6.16.0. |
Goal
Currently, a context is only set on DelayedJob errors if a display_name is set on the job, which is not always the case. The default context that it falls back to (top stack trace line) doesn't seem useful.
The default results in errors like
ActiveRecord::StatementInvalid app/models/foo.rb:100
. It does not indicate which job is failing, so one cannot understand the problem at glance in the dashboard.Other integrations attempt to set a useful default context, see:
bugsnag-ruby/lib/bugsnag/integrations/resque.rb
Line 46 in 56f387e
bugsnag-ruby/lib/bugsnag/middleware/rails3_request.rb
Line 19 in 56f387e
I think it would make sense to use the job name as the default context. This is consistent with other error monitoring tools I've used.
Changeset
Tests
Tests pass without changes. Let me know if you think additional coverage is needed.
Discussion
Alternative Approaches
I first tried defining a before_notify_callback to override the context, but that did not work.
Outstanding Questions
Linked issues
Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for:
@snmaynard @Cawllec